Skip to content

Conversation

@Yaraslaut
Copy link
Member

Closes #403

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modifies the Field class to have the modified flag set to true by default (instead of false), and adds logic in DataMapper to set the modified flag to false when records are loaded from the database. This change aims to distinguish between newly constructed records and records loaded from the database. The PR also adds support for creating records with user-defined primary keys.

Key changes:

  • Field's _modified flag now defaults to true instead of false
  • Added SetModifiedState() method to DataMapper to set modified flags after loading
  • Modified the Create method to check if a primary key value is set before auto-assigning
  • Added explicit conversion operator to Field class

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File Description
src/Lightweight/DataMapper/Field.hpp Changed default modified flag to true, added explicit conversion operator, made comparison operators constexpr
src/Lightweight/DataMapper/DataMapper.hpp Added SetModifiedState() method, modified Create() to check for pre-set primary keys, added SetModifiedState() calls in all Query methods
src/tests/DataMapper/Entities.hpp Added EntryWithIntPrimaryKey test entity
src/tests/DataMapper/CreateTests.cpp Added test case for creating records with predefined primary keys

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/Lightweight/DataMapper/DataMapper.hpp:1341

  • The new logic checks if the primary key value equals the default value (ValueType{}) to decide whether to auto-assign. This creates a problem: if a user explicitly wants to set an auto-assign primary key to its default value (e.g., 0 for integers), the auto-assignment logic will still trigger and overwrite the user's value. The old logic using IsModified() correctly distinguished between "not set" and "explicitly set to default value". With the new default of _modified = true, you should check if the value is the default AND the field is not modified, or find another way to distinguish these cases.
                    else if constexpr (requires { ValueType {} + 1; })
                    {
                        if (record.[:el:].Value() == ValueType {})
                        {
                            auto maxId = SqlStatement { _connection }.ExecuteDirectScalar<ValueType>(std::format(
                                R"sql(SELECT MAX("{}") FROM "{}")sql", FieldNameOf<el>, RecordTableName<Record>));
                            record.[:el:] = maxId.value_or(ValueType {}) + 1;
                        }
                    }

src/Lightweight/DataMapper/DataMapper.hpp:1364

  • The new logic checks if the primary key value equals the default value (ValueType{}) to decide whether to auto-assign. This creates a problem: if a user explicitly wants to set an auto-assign primary key to its default value (e.g., 0 for integers), the auto-assignment logic will still trigger and overwrite the user's value. The old logic using IsModified() correctly distinguished between "not set" and "explicitly set to default value". With the new default of _modified = true, you should check if the value is the default AND the field is not modified, or find another way to distinguish these cases.
            else if constexpr (requires { ValueType {} + 1; })
            {
                if (primaryKeyField.Value() == ValueType {})
                {
                    auto maxId = SqlStatement { _connection }.ExecuteDirectScalar<ValueType>(
                        std::format(R"sql(SELECT MAX("{}") FROM "{}")sql",
                                    FieldNameAt<PrimaryKeyIndex, Record>,
                                    RecordTableName<Record>));
                    primaryKeyField = maxId.value_or(ValueType {}) + 1;
                }
            }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/Lightweight/DataMapper/DataMapper.hpp:1340

  • The new logic checks if the primary key value equals the default-constructed value (e.g., 0 for int) to determine whether to auto-assign. This means that if a user explicitly sets an auto-assign primary key to its default value (like setting an int primary key to 0), it will still be auto-assigned rather than using the explicitly set value. Consider if this is the intended behavior, as it may be surprising for users who want to use 0 as a valid primary key value.
                        if (record.[:el:].Value() == ValueType {})
                        {
                            auto maxId = SqlStatement { _connection }.ExecuteDirectScalar<ValueType>(std::format(
                                R"sql(SELECT MAX("{}") FROM "{}")sql", FieldNameOf<el>, RecordTableName<Record>));
                            record.[:el:] = maxId.value_or(ValueType {}) + 1;
                        }

src/Lightweight/DataMapper/DataMapper.hpp:1363

  • The new logic checks if the primary key value equals the default-constructed value (e.g., 0 for int) to determine whether to auto-assign. This means that if a user explicitly sets an auto-assign primary key to its default value (like setting an int primary key to 0), it will still be auto-assigned rather than using the explicitly set value. Consider if this is the intended behavior, as it may be surprising for users who want to use 0 as a valid primary key value.
                if (primaryKeyField.Value() == ValueType {})
                {
                    auto maxId = SqlStatement { _connection }.ExecuteDirectScalar<ValueType>(
                        std::format(R"sql(SELECT MAX("{}") FROM "{}")sql",
                                    FieldNameAt<PrimaryKeyIndex, Record>,
                                    RecordTableName<Record>));
                    primaryKeyField = maxId.value_or(ValueType {}) + 1;
                }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@christianparpart christianparpart merged commit 3c47aeb into master Jan 14, 2026
16 checks passed
@christianparpart christianparpart deleted the fix/modified_flag branch January 14, 2026 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AutoAssign primary key

3 participants